Skip to content

Conversation

RubMaker
Copy link

  • Before adding new features and new modules, please go to issues to submit the relevant feature description first.
  • Write good commit messages and use the same coding conventions as the rest of the project.
  • Please commit code to dev branch and we will merge into master branch in feature
  • Ensure your edited codes with four spaces instead of TAB.

  • 增加新特性和新模块之前,请先到issues提交相关特性说明,经过讨论评估确认后,再进行相应的代码提交,避免做无用工作。
  • 编写友好可读的提交信息,并使用与工程代码相同的代码规范,代码请用4个空格字符代替tab缩进。
  • 请提交代码到dev分支,如果通过,我们会在特定时间合并到master分支上。
  • 为了规范化提交日志的格式,commit消息,不要用中文,请用英文描述。

Copy link
Contributor

Summary of Changes

Hello @RubMaker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the xmake build system by adding robust support for AppImage packaging. It provides a streamlined workflow for creating self-contained Linux applications, handling everything from tool detection and dependency resolution to the generation of necessary AppImage metadata and the final executable. This feature simplifies application distribution on Linux by minimizing dependency issues and installation complexities.

Highlights

  • AppImage Packaging Support: Introduced comprehensive support for packaging applications into the AppImage format, enabling self-contained Linux executables.
  • Tool Detection and Integration: Added new modules to detect and integrate external tools like appimagetool, linuxdeploy, and linuxdeployqt for efficient AppImage creation.
  • Automated Dependency Collection: Implemented logic for automatic dependency collection, specifically tailored for Qt projects using linuxdeployqt, and a generic fallback using linuxdeploy or manual ldd analysis.
  • New xpack APIs: Added xpack.set_iconname and xpack.set_appimage_tool APIs to allow configuration of AppImage properties within xmake build scripts.
  • AppDir Structure and Metadata Generation: Developed functions to create the standard AppDir structure, generate .desktop files, AppRun scripts, and manage application icons, ensuring proper AppImage metadata.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces AppImage packaging support, a valuable addition. The implementation is well-structured, featuring a robust fallback mechanism for dependency collection. I've identified a couple of critical areas where the code could fail in certain environments, along with several opportunities to enhance error reporting and clarity. My comments focus on improving the robustness and maintainability of the new packaging logic.

Comment on lines 44 to 60
local homedir = os.getenv("HOME")
local paths = {
"/usr/local/bin/linuxdeploy",
"/usr/bin/linuxdeploy",
"/opt/linuxdeploy/linuxdeploy",
path.join(os.tmpdir(), "linuxdeploy"),
path.join(homedir, "Downloads/linuxdeploy"),
path.join(homedir, "downloads/linuxdeploy"),
path.join(homedir, ".local/bin/linuxdeploy"),
path.join(homedir, "bin/linuxdeploy"),
"/snap/bin/linuxdeploy",
"/var/lib/flatpak/exports/bin/linuxdeploy",
path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeploy"),
path.join(os.curdir(), "linuxdeploy"),
path.join(os.curdir(), "tools/linuxdeploy"),
path.join(os.curdir(), "bin/linuxdeploy")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There's a potential crash here if the HOME environment variable is not set. os.getenv("HOME") would return nil, and path.join(nil, ...) will likely cause an error when constructing the paths table. To prevent this, you should only attempt to construct paths using homedir after verifying it's not nil.

        local homedir = os.getenv("HOME")
        local paths = {
            "/usr/local/bin/linuxdeploy",
            "/usr/bin/linuxdeploy",
            "/opt/linuxdeploy/linuxdeploy",
            path.join(os.tmpdir(), "linuxdeploy"),
            "/snap/bin/linuxdeploy",
            "/var/lib/flatpak/exports/bin/linuxdeploy",
            path.join(os.curdir(), "linuxdeploy"),
            path.join(os.curdir(), "tools/linuxdeploy"),
            path.join(os.curdir(), "bin/linuxdeploy")
        }
        if homedir then
            table.insert(paths, path.join(homedir, "Downloads/linuxdeploy"))
            table.insert(paths, path.join(homedir, "downloads/linuxdeploy"))
            table.insert(paths, path.join(homedir, ".local/bin/linuxdeploy"))
            table.insert(paths, path.join(homedir, "bin/linuxdeploy"))
            table.insert(paths, path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeploy"))
        end

Comment on lines 45 to 62
local homedir = os.getenv("HOME")

local paths = {
"/usr/local/bin/linuxdeployqt",
"/usr/bin/linuxdeployqt",
"/opt/linuxdeployqt/linuxdeployqt",
path.join(os.tmpdir(), "linuxdeployqt"),
path.join(homedir, "Downloads/linuxdeployqt"),
path.join(homedir, "downloads/linuxdeployqt"),
path.join(homedir, ".local/bin/linuxdeployqt"),
path.join(homedir, "bin/linuxdeployqt"),
"/snap/bin/linuxdeployqt",
"/var/lib/flatpak/exports/bin/linuxdeployqt",
path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeployqt"),
path.join(os.curdir(), "linuxdeployqt"),
path.join(os.curdir(), "tools/linuxdeployqt"),
path.join(os.curdir(), "bin/linuxdeployqt")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to find_linuxdeploy.lua, this code is susceptible to a crash if the HOME environment variable is not set. os.getenv("HOME") will return nil, causing path.join(nil, ...) to fail. You should ensure homedir is valid before using it to construct paths.

        local homedir = os.getenv("HOME")
        local paths = {
            "/usr/local/bin/linuxdeployqt",
            "/usr/bin/linuxdeployqt",
            "/opt/linuxdeployqt/linuxdeployqt",
            path.join(os.tmpdir(), "linuxdeployqt"),
            "/snap/bin/linuxdeployqt",
            "/var/lib/flatpak/exports/bin/linuxdeployqt",
            path.join(os.curdir(), "linuxdeployqt"),
            path.join(os.curdir(), "tools/linuxdeployqt"),
            path.join(os.curdir(), "bin/linuxdeployqt")
        }
        if homedir then
            table.insert(paths, path.join(homedir, "Downloads/linuxdeployqt"))
            table.insert(paths, path.join(homedir, "downloads/linuxdeployqt"))
            table.insert(paths, path.join(homedir, ".local/bin/linuxdeployqt"))
            table.insert(paths, path.join(homedir, "bin/linuxdeployqt"))
            table.insert(paths, path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeployqt"))
        end

Comment on lines 1 to 3
{
"deviceId": "1d865d85971033dbc6e2146fc84e4bab560676c51efb52db347df5c3d503bafb"
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This file appears to contain a user-specific or machine-specific device ID. Such files should not be committed to the version control system as they can expose sensitive information and are not relevant to other developers. Please remove this file from the repository and add .marscode/ to your .gitignore file to prevent it from being accidentally committed in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里没改

function _get_appimagetool()
local appimagetool = find_tool("appimagetool")
if not appimagetool then
assert(appimagetool, "appimagetool need to be downloaded!")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The assert message "appimagetool need to be downloaded!" could be more instructive for the user. A clearer message would help them resolve the issue faster.

        assert(appimagetool, "appimagetool not found. Please install it and ensure it is in your PATH.")

Comment on lines 118 to 120
if not ok then
return false
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The err variable returned by os.iorunv is not used. Logging this error when ok is false would provide valuable information for debugging why linuxdeployqt failed.

    if not ok then
        wprint("Failed to run linuxdeployqt: %s", err)
        return false
    end

Comment on lines +461 to +250
if not ok then
return false
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The err variable from os.iorunv is ignored. It would be beneficial for debugging to log the error message if the linuxdeploy command fails.

    if not ok then
        wprint("Failed to run linuxdeploy: %s", err)
        return false
    end

if linuxdeployqt then
deps_collected = _collect_qt_deps_with_linuxdeployqt(package, appdir, linuxdeployqt)
else
print("linuxdeployqt not available, will try fallback methods")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using wprint (warning print) instead of print for this message would be more consistent with how warnings are handled elsewhere in the file and would make it clearer to the user that this is a non-critical fallback situation.

            wprint("linuxdeployqt not available, will try fallback methods")

if linuxdeploy then
deps_collected = _collect_deps_with_linuxdeploy(package, appdir, linuxdeploy)
else
print("linuxdeploy not available")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency and better visibility, consider using wprint to log this warning, similar to other warnings in this script. Also, adding context that a fallback will be attempted would be helpful.

            wprint("linuxdeploy not available, will try fallback methods")

-- set nsis display icon
"xpack.set_nsis_displayicon",
-- set appimage icon name
"xpack.set_iconname",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也是

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里还没改

local program = find_program("linuxdeploy", opt)

-- if not found, check common installation paths
if not program then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里都没改

-- You may obtain a copy of the License at
--
-- http://www.apache.org/licenses/LICENSE-2.0
--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qt相关的先删了


-- if not found, check common installation paths
if not program then
local paths = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也没改,看下之前的 review 信息

end

-- collect Qt dependencies with linuxdeployqt (rewritten)
function _collect_qt_deps_with_linuxdeployqt(package, appdir, linuxdeployqt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里都没删,qt 的

-- limitations under the License.
--
-- Copyright (C) 2015-present, Xmake Open Source Community.
--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

删了

end
return appimagetool
end
-- get linuxdeploy tool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不要挤一起,适当换行下。。

-- get linuxdeploy tool
function _get_linuxdeploy()
local linuxdeploy = assert(find_tool("linuxdeploy"), "linuxdeploy not found!Pease install it in Downloads folder.")
return linuxdeploy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

删了

end
-- execute install commands
for _, cmd in ipairs(installcmds) do
os.exec(cmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也没改,看下最早我的 review ,直接走 runcmds

@RubMaker RubMaker force-pushed the feat-appimage branch 2 times, most recently from 1ef0c63 to 57bd2a6 Compare September 29, 2025 23:12
@waruqi
Copy link
Member

waruqi commented Oct 7, 2025

这里还有一些 review 没有处理

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


There are still some reviews that are not processed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants